Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Collator overseer builder unification #3335

Merged
merged 15 commits into from
Feb 28, 2024
Merged

Collator overseer builder unification #3335

merged 15 commits into from
Feb 28, 2024

Conversation

maksimryndin
Copy link
Contributor

@maksimryndin maksimryndin commented Feb 15, 2024 •
edited
Loading

resolve #3116

a follow-up on #3061 (review):

  • reuse collator overseer builder for polkadot-node and collator
  • run zombienet test (0001-parachains-smoke-test.toml)
  • make wasm build errors more user-friendly for an easier problem detection when using different toolchains in Rust

@maksimryndin
Copy link
Contributor Author

@skunert @s0me0ne-unkn0wn
Proposed labels: T0-node, T9-cumulus, I4-refactor, R0-silent

@maksimryndin maksimryndin marked this pull request as ready for review February 15, 2024 13:20
@skunert skunert self-requested a review February 16, 2024 08:34
@skunert skunert added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. I4-refactor Code needs refactoring. T9-cumulus This PR/Issue is related to cumulus. labels Feb 19, 2024
@skunert
Copy link
Contributor

skunert commented Feb 19, 2024

Thank you for looking into this!

IMO we could go even further and remove special treatment of the ChainApiSubsystem.
So basically what I am suggesting:

  1. Change the requirements for RuntimeClient in overseer.rs to RuntimeClient: RuntimeApiSubsystemClient + ChainApiBackend + AuxStore + 'static
  2. Implement AuxStore and HeaderBackend for DefaultSubsystemClient
  3. Instantiate DefaultSubsystemClient here

    polkadot-sdk/polkadot/node/service/src/lib.rs

    Line 1085 in 2a32b5c

    let (overseer, overseer_handle) = overseer_gen

We would get rid of the offfchain_transaction_pool_factory arg here and can remove generic_collator_overseer_builder. Does that make sense?

@maksimryndin
Copy link
Contributor Author

maksimryndin commented Feb 19, 2024 •
edited
Loading

Thank you for looking into this!

IMO we could go even further and remove special treatment of the ChainApiSubsystem. So basically what I am suggesting:

1. Change the requirements for `RuntimeClient` in `overseer.rs` to `RuntimeClient: RuntimeApiSubsystemClient + ChainApiBackend + AuxStore + 'static`

2. Implement `AuxStore` and `HeaderBackend` for `DefaultSubsystemClient`

3. Instantiate `DefaultSubsystemClient` here https://github.com/paritytech/polkadot-sdk/blob/2a32b5c9379e6daefd2061a8fb5b4d972d3460a1/polkadot/node/service/src/lib.rs#L1085

We would get rid of the offfchain_transaction_pool_factory arg here and can remove generic_collator_overseer_builder. Does that make sense?

@skunert An excellent idea! Thank you! I believe I've got it but have some compilation errors, will try to resolve tomorrow

skunert
skunert approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice! Thanks a lot!

I left some last suggestions, otherwise looks good.

polkadot/node/subsystem-types/src/runtime_client.rs Outdated Show resolved Hide resolved
polkadot/node/subsystem-types/src/runtime_client.rs Outdated Show resolved Hide resolved
polkadot/node/service/src/overseer.rs Outdated Show resolved Hide resolved
polkadot/node/service/src/overseer.rs Outdated Show resolved Hide resolved
@skunert skunert requested review from a team and s0me0ne-unkn0wn February 21, 2024 16:51
ordian
ordian approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty! LGTM

substrate/utils/wasm-builder/src/prerequisites.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes seem unrelated, but looking welcome and good
for the future, i'd prefer this to be submitted via a different PR, but let's keep this one here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry, I encountered some issues during the setup on arm64 Linux so I decided to make the messages more user-friendly :)

AndreiEres
AndreiEres approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@AndreiEres AndreiEres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@s0me0ne-unkn0wn s0me0ne-unkn0wn added this pull request to the merge queue Feb 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 27, 2024
@ordian ordian added this pull request to the merge queue Feb 28, 2024
Merged via the queue into paritytech:master with commit 7ec0b87 Feb 28, 2024
129 of 130 checks passed
ordian added a commit that referenced this pull request Feb 28, 2024
@ordian
Merge remote-tracking branch 'origin/master' into ao-collator-parent-…
8b70d0f 
…head-data

* origin/master: (51 commits)
  Runtime Upgrade ref docs and Single Block Migration example pallet  (#1554)
  Collator overseer builder unification (#3335)
  Introduce storage attr macro `#[disable_try_decode_storage]` and set it on `System::Events` and `ParachainSystem::HostConfiguration` (#3454)
  Add Polkadotters bootnoders per IBP application (#3423)
  Add documentation around FRAME Origin (#3362)
  Bridge zombienet tests: Check amount received at destination (#3490)
  Snowbridge benchmark tests fix (#3424)
  fix(zombienet): increase timeout in download artifacts (#3376)
  Cleanup String::from_utf8 (#3446)
  [prdoc] Validate crate names (#3467)
  Limit max execution time for `test-linux-stable` CI jobs (#3483)
  Introduce Notification block pinning limit (#2935)
  frame-support: Improve error reporting when having too many pallets (#3478)
  add Encointer as trusted teleporter for Westend (#3411)
  [pallet-xcm] Adjust benchmarks (teleport_assets/reserve_transfer_assets) not relying on ED (#3464)
  Add more debug logs to understand if statement-distribution misbehaving (#3419)
  Remove redundant parachains assigner pallet. (#3457)
  Use generic hash for runtime wasm in resolve_state_version_from_wasm (#3447)
  Runtime: allow backing multiple candidates of same parachain on different cores  (#3231)
  Bridge zombienet tests: move all "framework" files under one folder (#3462)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@maksimryndin @ordian @s0me0ne-unkn0wn
Collator overseer builder unification (paritytech#3335)
4fbe140 
resolve paritytech#3116

a follow-up on
paritytech#3061 (review):

- [x] reuse collator overseer builder for polkadot-node and collator
- [x] run zombienet test (0001-parachains-smoke-test.toml)
- [x] make wasm build errors more user-friendly for an easier problem
detection when using different toolchains in Rust

---------

Co-authored-by: ordian <[email protected]>
Co-authored-by: s0me0ne-unkn0wn <[email protected]>
This was referenced Jun 5, 2024
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreiEres AndreiEres AndreiEres approved these changes

@skunert skunert skunert approved these changes

@ordian ordian ordian approved these changes

@koute koute Awaiting requested review from koute koute is a code owner

@s0me0ne-unkn0wn s0me0ne-unkn0wn Awaiting requested review from s0me0ne-unkn0wn

Assignees
No one assigned
Labels
I4-refactor Code needs refactoring. R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use common overseer construction codepath for polkadot node + collator
5 participants
@maksimryndin @skunert @ordian @AndreiEres @s0me0ne-unkn0wn